Skip to content

Delete empty dirs in baseline-accept task#2924

Closed
jakebailey wants to merge 2 commits intomainfrom
jabaile/baseline-accept-delete-empty
Closed

Delete empty dirs in baseline-accept task#2924
jakebailey wants to merge 2 commits intomainfrom
jabaile/baseline-accept-delete-empty

Conversation

@jakebailey
Copy link
Member

The empty dirs are pretty annoying sometimes when comparing. Just have the accept task delete them.

Copilot AI review requested due to automatic review settings February 27, 2026 00:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the repo’s baseline-accept hereby task to clean up empty baseline directories left behind after accepting deletions, reducing noise when comparing baseline trees.

Changes:

  • Added a helper to walk upward and remove empty directories after baseline deletions.
  • Extended baselineAcceptTask to run the empty-directory cleanup for both local and reference baseline trees.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment on lines +908 to +910
// If root itself is entirely empty, there's nothing to collect
// (we exclude root). Otherwise, visit already pushed the right subtrees.
visit(root);
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findEmptySubtrees() doesn’t currently handle the case where root’s entire subtree is empty-but-nontrivial (i.e. root contains only empty directories). In that scenario visit(root) returns true and the function returns [], so no empty directories are removed even though there are empty trees under root (contradicting the doc comment / PR goal). Consider: if visit(root) returns true, collect and return root’s direct child directories (or otherwise ensure empty subtrees under an empty root are still returned), while still not deleting root itself.

Suggested change
// If root itself is entirely empty, there's nothing to collect
// (we exclude root). Otherwise, visit already pushed the right subtrees.
visit(root);
// If root itself is entirely empty, we still want to collect the
// top-most empty subtrees under it (its direct child directories),
// while excluding root itself. Otherwise, visit already pushed
// the right subtrees into result.
const rootIsEmpty = visit(root);
if (rootIsEmpty) {
/** @type {import("fs").Dirent[]} */
let entries;
try {
entries = fs.readdirSync(root, { withFileTypes: true });
}
catch {
// If we can't read root at this point, just return whatever
// was already collected.
return result;
}
for (const entry of entries) {
if (entry.isDirectory()) {
result.push(path.join(root, entry.name));
}
}
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants